Return signing key in SignedEnvelope.payload #2522
Conversation
| /// It is the caller's responsibility to check that the signing key is what | ||
| /// is expected. For example, checking that the signing key is from a | ||
| /// certain peer. |
There was a problem hiding this comment.
How about enforcing this constraint via #[must_use]? Something along the lines of the playground below:
There was a problem hiding this comment.
Oh excellent! I was wondering if there was a way I could leverage #[must_use]. Thanks!
There was a problem hiding this comment.
As far as I can tell, unfortunately one can not do it on an anonymous tuple. E.g. instead of the additional struct I would prefer to do:
fn foo() -> Result<(a, #[must_use] b), c> {}There was a problem hiding this comment.
Yeah that was my first thought, but I hadn't considering using a struct instead.
There was a problem hiding this comment.
Sorry, the above suggestion actually does not work. The compiler reports the field not being used in general, rather than not being used in this particular case.
Also nesting #[must_use] structs doesn't work see rust-lang/rust#39524.
Also @MarcoPolo pointed out that users would likely do something along the lines of let (payload, _) = payload().handle_error(); which would thus circumvent the #[must_use].
Sorry for the noise here.
There was a problem hiding this comment.
We could do something like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=92e6fc6cf971873eb590d9ada72f85fa
But I am not sure if it is useful. Assigning _ will silence the must_use warning ...
There was a problem hiding this comment.
Unfortunately assigning the return value to anything and not using the key value will also satisfy the must_use lint. e.g. let data = foo().unwrap();
core/src/signed_envelope.rs
Outdated
| /// | ||
| /// It is the caller's responsibility to check that the signing key is what | ||
| /// is expected. For example, checking that the signing key is from a | ||
| /// certain peer. | ||
| pub fn payload( |
There was a problem hiding this comment.
| pub fn payload( | |
| pub fn payload_and_signing_key( |
What do you think of renaming the method as well? If I recall correctly you suggested this in your initial vulnerability report, right?
There was a problem hiding this comment.
Changed in ef3afcd
|
I suggest we move forward and merge here. @MarcoPolo given that this is a breaking change, would you mind adding a changelog entry to |
Context
Fixes #2511.
Another solution would be to pass in a closure as a key validation function to
payload. I originally tried this, but quickly noticed that it made thePeerRecorduse case awkward. This is because the peer id we are validating against is inside the payload. So the closure would need access to the payload. If you passed in the payload to the closure, then you would need to parse the payload twice (or awkwardly mutate an outer variable).So instead, I return the signing key from the
payloadmethod.If I'm misunderstanding something @thomaseizinger please let me know.